Cryptographic function interface #46

IbraheemYSaleh · 2022-01-04T03:30:09Z

No description provided.

…vice

…crypt

…ation/refactor

IbraheemYSaleh · 2022-01-04T18:32:44Z

I'm still dealing with some issues when running the unit tests... They pass just fine individually, but when run together, cause erratic behavior -- Since we're time crunched now, I've requested your review even before I trace down this bug as discussed in the meeting.

I imagine the fix won't require significant code changes, so reviewing the interface & refactor as it is now for any changes that you think are necessary should be mostly valid & worthwhile.

rjbrown2 · 2022-01-04T19:16:53Z

...src_cryptography/src_kmc_crypto_service/cryptography_interface_kmc_crypto_service.template.c

+** Global Variables
+*/
+// Cryptography Interface
+static CryptographyInterfaceStruct cryptography_if_struct;


Should these globals be here, or should they be declared in the crypto_interface.c ? --Per J.Lucas

It's not a global actually, it's a static (local/module) variable... Sorry, I didn't update the comment here, but I did in the libgcrypt interface file:
https://github.com/nasa/CryptoLib/pull/46/files#diff-8455637f5a51288fb98808edae34ef7ba4faf362e80d728810678ec2701f1399R51

This is the local struct that contains all of the function implementations for the cryptography interface. It is defined with different function pointers depending on which interface is being used, and returned to the core application. This should be here and is really central to how the interface layer is implemented, IE, via these locally defined structs with different function pointers.

rjbrown2 · 2022-01-04T19:25:19Z

src/src_cryptography/src_libgcrypt/cryptography_interface_libgcrypt.template.c

+CryptographyInterface get_cryptography_interface_libgcrypt(void)
+{
+    cryptography_if_struct.cryptography_config = cryptography_config;
+    cryptography_if_struct.cryptography_init = cryptography_init;
+    cryptography_if_struct.get_ek_ring = get_ek_ring;
+    cryptography_if_struct.cryptography_shutdown = cryptography_shutdown;
+    cryptography_if_struct.cryptography_encrypt = cryptography_encrypt;
+    cryptography_if_struct.cryptography_decrypt = cryptography_decrypt;
+    cryptography_if_struct.cryptography_authenticate = cryptography_authenticate;
+    cryptography_if_struct.cryptography_validate_authentication = cryptography_validate_authentication;
+    cryptography_if_struct.cryptography_aead_encrypt = cryptography_aead_encrypt;
+    cryptography_if_struct.cryptography_aead_decrypt = cryptography_aead_decrypt;
+    return &cryptography_if_struct;


Should we distinguish these function names between the different types? eg kmc_cryptography_config

We could do that, but I think that would actually make working with the code base somewhat trickier... If we ever need to update the interface, we generally need to update it in all the places where the functions are defined -- if they have different names, search and replace won't work, and you'll need to manually map out what goes to what in your IDE.

Having a unique name here doesn't add much, just knowing that the functions are static (IE locally defined in this code file) should be enough of an indication about what's going on.

rjbrown2 · 2022-01-04T19:43:42Z

src/src_main/crypto_tc.c

+                                                           aad_len, // Length of AAD
+                                                           (sa_ptr->est==1),
+                                                           (sa_ptr->ast==1),
+                                                           (sa_ptr->ast==1)


Remove duplicated pointer

Should be (sa_ptr->aad==1) ? With aad instead of ast?

I think I might agree with you, these three parameters refer to the following booleans in the interface function:
uint8_t decrypt_bool, uint8_t authenticate_bool,
uint8_t aad_bool)

The question is, will you ever provide AAD, but not authenticate? Or Authenticate, but not provide AAD? If the answer is no to both questions, then we can combine those both into a single authenticate_bool.

For some reason during my refactor, I thought I ran into one such case:
https://github.com/nasa/CryptoLib/pull/46/files#diff-680e5a3e45c21aa3a202f9dcff50de85c32aad656eaae798a6052c7495c7df59R106

But that may be incorrect (there was also the issue with it doing authenticate then encrypt, instead of encrypt then authenticate). I will investigate some more.

For now, I think it's OK to keep the interface function as having different controls for authentication vs AAD related calls... In the case of TC, the presence of AAD is determined by the AST flag in the SA... I don't know about the TM & key management cases, as linked above, it seems to vary, but that logic may be wrong. For the purposes of this initial interface implementation, it should be OK to keep it separate, as it is now.

rjbrown2 · 2022-01-04T19:51:27Z

src/src_main/crypto_tc.c

+                                                   aad_len, // length of AAD
+                                                   (sa_ptr->est==1),
+                                                   (sa_ptr->ast==1),
+                                                   (sa_ptr->ast==1)


Same question as line 460

rjbrown2

I think this is ok. As suggested on slack, any changes, due to time constraints can be put in as issues for later.

szemerick

I like the interface. Simple and straightforward. I will followup with our guys about additional testing.

iysaleh and others added 5 commits December 28, 2021 10:32

Minor SQL fixes and query updates due to SA changes

2bb7a03

Add Cryptography Interface shell code for libcrypt and KMC crypto ser…

9c5e252

…vice

Add initial cryptography interface and libgcrypt refactor for AEAD en…

1df04d8

…crypt

Add Cryptography Interface aead_decrypt logic and libgcrypt implement…

2342650

…ation/refactor

Properly configure cryptography interface and small debug improvements

3fde1b1

IbraheemYSaleh requested review from jlucas9, dccutrig, rjbrown2 and szemerick January 4, 2022 18:30

rjbrown2 reviewed Jan 4, 2022

View reviewed changes

IbraheemYSaleh added 2 commits January 4, 2022 13:50

Add missing length calculation after interface refactor

81da563

Re-add ping pdu decryption validation (with auth check disabled)

d059e85

IbraheemYSaleh requested a review from rjbrown2 January 4, 2022 22:26

rjbrown2 approved these changes Jan 4, 2022

View reviewed changes

szemerick approved these changes Jan 5, 2022

View reviewed changes

IbraheemYSaleh merged commit 89ceed0 into collab_main Jan 5, 2022

IbraheemYSaleh deleted the CryptographicFunctionInterface branch January 5, 2022 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cryptographic function interface #46

Cryptographic function interface #46

IbraheemYSaleh commented Jan 4, 2022

IbraheemYSaleh commented Jan 4, 2022 •

edited

Loading

rjbrown2 Jan 4, 2022

IbraheemYSaleh Jan 4, 2022

rjbrown2 Jan 4, 2022

IbraheemYSaleh Jan 4, 2022

rjbrown2 Jan 4, 2022

rjbrown2 Jan 4, 2022

IbraheemYSaleh Jan 4, 2022 •

edited

Loading

IbraheemYSaleh Jan 4, 2022

rjbrown2 Jan 4, 2022

rjbrown2 left a comment

szemerick left a comment

Cryptographic function interface #46

Cryptographic function interface #46

Conversation

IbraheemYSaleh commented Jan 4, 2022

IbraheemYSaleh commented Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IbraheemYSaleh Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbrown2 left a comment

Choose a reason for hiding this comment

szemerick left a comment

Choose a reason for hiding this comment

IbraheemYSaleh commented Jan 4, 2022 •

edited

Loading

IbraheemYSaleh Jan 4, 2022 •

edited

Loading